Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adjust checking for internal and public errors #153

Merged
merged 6 commits into from
Sep 2, 2024

Conversation

CatchMe2
Copy link
Collaborator

Changes

Please describe

Checklist

  • Apply one of following labels; major, minor, patch or skip-release
  • I've updated the documentation, or no changes were necessary
  • I've updated the tests, or no changes were necessary

Copy link
Collaborator

@kibertoad kibertoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a second thought, symbols might not be the best fit here. will write a comment to explain

export class InternalError<T = ErrorDetails> extends Error {
readonly [Symbol.for(INTERNAL_ERROR_SYMBOL_KEY)] = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem with using a symbol here is that it might make the implementation more fragile than intended.

Imagine that node-core 11 throws an error somewhere deep in your dependencies, and your app is using node-core 12. those will be two separate imports, and each will have their own Symbol instance, so errors from one node-core version will no longer be recognized as errors from another node-core version.

plain string field would work better here. it will have worse encapsulation, but it's unlikely to cause any real issues

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel like having a plain text field would change anything about backward compatibility. This new field would be only available in node-code v12 so the same problem as for symbol.

The only difference between the Symbol and plain text is that Symbol won't be serialized in the case of JSON.stringify(err) or it won't be iterable with Object.entries(err) so that it will behave the same as node-core v11.

I think that instead, we should adjust how we check if the error is internal or not so we might still support the old way of checking for it like

 return isError(error) && (error[Symbol.for(INTERNAL_ERROR_SYMBOL_KEY)] === true || error.name === 'InternalError')

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a naive test, and within same realm, indeed, Symbol.for is using a global registry, so we receive same symbol for the same string, and everything works as expected.

Tomorrow I'll setup a test with separate realms and see if it still doesn't break.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already tested it:

import { createContext, runInContext } from 'node:vm';
import { isNativeError } from 'node:util/types'

const errContext = {};
createContext(errContext);

const errResult = runInContext(`new Error('Unknown')`, errContext);

console.log(errResult instanceof Error) // false
console.log(isNativeError(errResult)) // true


const symbolContext = {};
createContext(symbolContext);

const symbolResult = runInContext(`Symbol.for('ANY')`, errContext);

console.log(symbolResult === Symbol.for('ANY')) // true

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reproduction for the problem: lokalise/node-service-template#761

Copy link
Collaborator

@kibertoad kibertoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will test more later, but adjusting this later should be a pretty minor non-breaking change

@kibertoad kibertoad merged commit 7eca92a into main Sep 2, 2024
5 checks passed
@kibertoad kibertoad deleted the adjust-check-for-internal-and-public-error branch September 2, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants